Makes it easier to use custom Routes within WorkboxSW. #639
Conversation
|
@jeffposnick this seems like a reasonable solution to me. What was the original/current thinking on not including |
|
I don't remember the original thinking—@gauntface and I had some back and forth about it, but I'm not even sure which side each of us took 😄 It's slightly awkward to expose the We could expose a I guess my vote is for the factory method, but that can be implemented separately from this PR. |
| for (let item of object[parameter]) { | ||
| for (let property of expectedProperties) { | ||
| if (!(property in item)) { | ||
| throwError(message); |
There was a problem hiding this comment.
same as above, the message should be updated to handle the missing property
| import logHelper from '../../../../lib/log-helper.js'; | ||
| import normalizeHandler from './normalize-handler'; | ||
|
|
||
| const ROUTE_INTERFACE = ['match', 'handler', 'method']; |
There was a problem hiding this comment.
I'm not a huge fan of having this manually defined and it being defined away from the class it represents.
|
|
||
| for (let property of expectedProperties) { | ||
| if (!(property in object[parameter])) { | ||
| throwError(message); |
There was a problem hiding this comment.
Would it be worth adding the missing parameter to the message?
|
In the early versions we originally exposed it, but developer feedback was that it caused confusion have it exposed so closely to router. |
| @@ -97,9 +99,9 @@ class Router extends SWRoutingRouter { | |||
| if (capture.length === 0) { | |||
| throw ErrorFactory.createError('empty-express-string'); | |||
| } | |||
| route = new ExpressRoute({path: capture, handler}); | |||
| route = new ExpressRoute({path: capture, handler, method}); | |||
|
Discussed with @jeffposnick , we can move the capture argument to accept a function, this will serve as the 'match' input for a Route that will be constructed behinds the scenes. Sorry for this wrinkle.There is a largely re-think we should probably have about how we expose these values. |
|
@gauntface, to clarify, your proposal is to allow for the following? workboxSW.router.registerRoute(
// Capture
({ url, event }) => {
return true;
},
// Handler
({ event }) => {
return caches.match(event.request);
},
); |
|
That's correct, @mikefowler. I'm going to create a fresh PR that takes that approach. |
R: @addyosmani @gauntface
CC: @jmtt89 @mikefowler
This is something of a straw-man PR, and feedback is welcome.
This partially addresses #385 and #636, but I'm not sure that I'd call it a "fix", so I'm inclined to leave them open.
Still, it should make it easier to accomplish what @jmtt89 was trying to do, by letting you use a method other than
'GET'when callingworkboxSW.router.registerRoute().It also provides an escape hatch granting greater flexibility by letting developers import and use
Routeobjects created from theworkbox.routingnamespace, by relaxing the assertions to just check whether the object passed toregisterRoute()has the properties that aRouteshould have, not necessarily whether it's a subclass of a specific definition of aRouteobject.This PR doesn't attempt to add in a factory method for creating new
Routeobjects exposed viaworkboxSW.router, although that's an idea that's been discussed in the past and might still be on the table.